Skip to content

Add more numeric types - #271

Merged
isublimity merged 5 commits into
smi2:masterfrom
RemcoSmitsDev:feat/extend-clickhouse-types
Sep 25, 2026
Merged

isublimity merged 5 commits into
smi2:masterfrom
RemcoSmitsDev:feat/extend-clickhouse-types

Conversation

@RemcoSmitsDev

Copy link
Copy Markdown
Contributor

No description provided.

@RemcoSmitsDev
RemcoSmitsDev marked this pull request as draft September 17, 2026 12:33
@sander-hash

Copy link
Copy Markdown
Contributor

You are missing the Uint32 type

@sander-hash

Copy link
Copy Markdown
Contributor

Should we make the numeric types with bytes 32 and below make INT since php supports this native?

@RemcoSmitsDev

Copy link
Copy Markdown
Contributor Author

Should we make the numeric types with bytes 32 and below make INT since php supports this native?

I’d keep the numeric type wrappers string-backed for consistency across all ClickHouse integer types. Although Int8, Int32, UInt8, UInt32 fit in a 64-bit PHP int, using strings keeps the API platform-independent and avoids changing behavior between small and large integer types.

@RemcoSmitsDev
RemcoSmitsDev marked this pull request as ready for review September 17, 2026 12:52
@isublimity

Copy link
Copy Markdown
Contributor

Review: tests fail on both supported ClickHouse versions

Thanks for the PR — the wrappers follow the existing pattern (UInt64/Int64) and PHPStan is clean. However, I ran the suites locally against CH 21.9 and CH 26.3.3.20, and ScalarNumericIntegrationTest fails on both (note: GitHub Actions hadn't run for this PR yet — I've approved the workflow, it will run on your next push):

1. Decimal32(9) cannot hold 1.1 (fails on both versions)
Decimal32(9) means precision = 9 and scale = 9, so only values |x| < 1 fit:

Decimal value is too big: 2 digits were read: 11e-1. Expected to read decimal with scale 9 and precision 9 (ARGUMENT_OUT_OF_BOUND)

Use a realistic scale, e.g. Decimal32(2).

2. Decimal > Float64 is unsupported on CH 21.9 (Decimal64/128/256 cases)

No operation greater between Decimal(18, 9) and Float64 (ILLEGAL_TYPE_OF_ARGUMENT)

CH 26 accepts it, 21.9 does not — and both must pass (phpunit-ch21.xml). Compare against a decimal literal, e.g. WHERE value > toDecimal64('1.1', 2).

3. Float32 representation breaks the comparison assertion
Float32 1.1 is stored as ≈ 1.10000002, which is greater than the Float64 literal 1.1, so WHERE value > 1.1 matches both rows and assertSame(1, $statement->count()) fails. Pick values where representation can't bite, e.g. insert 1.5/2.5 and compare > 2.

4. Please add input validation to fromString()
Type objects bypass ValueFormatter escaping and are interpolated into SQL raw, so Int32::fromString($untrustedInput) is an SQL-injection vector. For numeric wrappers validation is cheap:

  • integers: preg_match('/^-?\d+$/', $value)
  • floats/decimals: is_numeric($value)

throwing InvalidArgumentException otherwise. (The existing UInt64 has the same gap, but let's not multiply it by 18 new classes.)

5. Rebase needed
#269 has just been merged and touches todo.md and doc/types.md — conflicts in those two files are expected when you rebase on master.

Happy to merge once the above is addressed — the scope of the PR itself is very welcome.

isublimity and others added 2 commits September 25, 2026 08:08
…rong Decimal doc claim

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Int*/UInt* fromString() now require an integer string (^-?\d+$),
  Float*/Decimal* require is_numeric() — Type objects bypass
  ValueFormatter escaping, so unvalidated input was an SQL injection
  vector; unit tests cover the rejected inputs
- Decimal test columns use scale 2: Decimal32(9) cannot hold 1.1
  (precision 9 = scale 9 leaves no integer digits)
- comparison literal is CAST to the column type: Decimal vs Float64 is
  unsupported on CH 21, and Float32 1.1 != Float64 1.1 representation

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@isublimity

Copy link
Copy Markdown
Contributor

I went ahead and pushed the fixes so we can ship this (merged master + two commits):

  • Merged master — resolved todo.md against the English translation from Translate todo from russian to english #268, marked Phase 1 done; also dropped the doc claim that Decimal::fromString() accepts precision/scale (it takes a single argument).
  • fromString() validation — Int*/UInt* now require ^-?\d+$, Float*/Decimal* require is_numeric(), throwing InvalidArgumentException otherwise; unit tests cover SQL-injection-shaped inputs.
  • Integration test — Decimal columns use scale 2 (Decimal32(9) can't hold 1.1), and the comparison literal is CAST(... AS <column type>), which fixes both Decimal > Float64 on CH 21.9 and the Float32 representation mismatch.

Verified locally: CH 21.9 — 431 tests OK, CH 26.3 — 420 tests OK, PHPStan and PHPCS clean on the touched files. Thanks for the contribution!

@isublimity
isublimity merged commit 3bfe483 into smi2:master Sep 25, 2026
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants